PR Review: Remove all [Obsolete] members and migrate callers

Overall this is a clean, well-scoped removal PR that follows through on prior deprecation work. The migration from HasCount/HasLength to Count().IsEqualTo(N) / Length().IsEqualTo(N) is executed consistently across the entire codebase, and removing ObjectBag / Timing / RecordTiming completes an earlier breaking-change cycle properly.

A few observations worth flagging:

1. StringLengthWithInlineAssertionAssertion duplicates CollectionCountWithInlineAssertionAssertion almost exactly

The new class in TUnit.Assertions/Conditions/StringAssertions.cs is structurally identical to CollectionCountWithInlineAssertionAssertion<TCollection, TItem> in CollectionCountSource.cs. Both:

• Accept a Func<IAssertionSource<int>, Assertion<int>?> lambda
• Create a ValueAssertion<int> internally
• Catch failures and return a generic "was N" failure message
• Use "to satisfy X assertion" as their expectation string

This duplication means any future improvement (better failure messages, async evaluation, etc.) would have to be applied in two places. A shared base class or a helper method (e.g. InlineNumericAssertionHelper.Run(...)) would eliminate the duplication. The CollectionCountWithInlineAssertionAssertion pattern already exists and works — StringLengthWithInlineAssertionAssertion could simply extend or delegate to the same mechanism.

2. The new Length(lambda) overload silently swallows the inner assertion's failure message

When the lambda assertion fails, both the existing Count(lambda) and the new Length(lambda) implementations catch the exception and return a generic AssertionResult.Failed($"length was {_actualLength}"). This loses the inner assertion's error message entirely.

For example:

await Assert.That(str).Length(l => l.IsGreaterThan(10));
// Failure: "expected to satisfy length assertion but length was 3"
// vs. what the user wants: "expected length to be greater than 10 but received 3"

The CollectionCountWithInlineAssertionAssertion has the same issue — this PR is a good opportunity to fix it in both places by propagating the inner exception message rather than discarding it.

3. TimingEntry is now internal record but structurally identical to the deleted public Timing record

The deleted Timing record was:

public record Timing(string StepName, DateTimeOffset Start, DateTimeOffset End)
{
    public TimeSpan Duration => End - Start;
}

The new TimingEntry in TestContext.Output.cs is:

internal record TimingEntry(string StepName, DateTimeOffset Start, DateTimeOffset End)
{
    public TimeSpan Duration => End - Start;
}

This is intentional (it's now engine-internal) and correctly placed, but the TestExtensions.cs usage of testContext.Timings still iterates TimingEntry objects to populate TimingProperty for TRX reporting. The fact that TimingEntry lives in TUnit.Core but is only used by TUnit.Engine is fine, but worth verifying that no consumers in TUnit.Engine reference the deleted public Timing type by name (e.g. via ITestOutput.Timings which is now removed). The #pragma disable CS0618 suppressions being removed is a good sign that this is clean.

4. AssertionResult.Failed("value was null") vs. AssertionResult.Failed("collection was null") inconsistency in StringLengthWithInlineAssertionAssertion

The new class returns "value was null" for a null string, while the equivalent count assertion returns "collection was null". This is a minor wording inconsistency — either "string was null" or mirroring "value was null" across both would be more consistent.

Summary

The bulk of the diff (migrating test callsites) is correct and complete. The structural concern is the near-duplicate implementation of StringLengthWithInlineAssertionAssertion — it would be worth extracting the shared lambda-assertion execution pattern into a helper to avoid future drift. The silent failure message swallowing is a pre-existing issue in Count(lambda) that this PR inadvertently carries forward into Length(lambda) as well.

PR Review: Remove all [Obsolete] members and migrate callers (follow-up)

Comparing against the previous review:

Addressed from prior review ✅

Duplication of inline assertion logic: The extraction of InlineAssertionHelper.ExecuteInlineAssertionAsync() correctly eliminates the near-duplicate between CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion. The helper now also propagates ex.Message rather than a generic fallback string, which was the other open concern.

Remaining issues

1. LengthWrapper and CountWrapper are now dead code and should be deleted

HasLength() (no-arg overload) and HasCount() (no-arg overload) that returned these wrappers have been removed. There is no longer any way to construct a LengthWrapper or CountWrapper, yet both files remain with only doc-comment updates applied.

These classes should be deleted outright. Keeping them adds noise to the public API surface (they're public) and the dead code signals an incomplete cleanup. Their presence also means the residual "IsTypeOf is not supported after HasCount()" error messages in CountWrapper are permanently stale — further evidence they should go.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() is too vague

This produces failure messages like: "expected to satisfy length assertion but value was null". The equivalent CollectionCountWithInlineAssertionAssertion likely has the same issue. A better approach would be to lazily capture the expectation from the inner assertion after it has been evaluated — the inner Assertion<int> already has its own GetExpectation() which would produce something like "to be equal to 5". Forwarding that gives: "expected length to be equal to 5 but received 3", which is much clearer.

3. InlineAssertionHelper is typed to int only

The helper signature is:

This works today since both count and length are int, but it rules out reuse for long, double, etc. without a second overload. Making it generic (ExecuteInlineAssertionAsync<T>) would cost nothing now and prevent a copy-paste if another numeric type is needed later.

Minor

• StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string, while the count equivalent uses "collection was null". "string was null" would be more specific and consistent with the count wording.

Overall

The bulk of this PR — migrating 80+ callsites and removing the public obsolete API surface — is well-executed and complete. The main actionable items are deleting the now-unreachable CountWrapper/LengthWrapper classes and improving the GetExpectation() message quality in StringLengthWithInlineAssertionAssertion.

PR Review: Remove all [Obsolete] members and migrate callers (follow-up)

Comparing against the previous review:

Addressed from prior review ✅

Duplication of inline assertion logic: The extraction of InlineAssertionHelper.ExecuteInlineAssertionAsync() correctly eliminates the near-duplicate between CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion. The helper now also propagates ex.Message rather than a generic fallback string, which was the other open concern.

Remaining issues

1. LengthWrapper and CountWrapper are now dead code and should be deleted

HasLength() (no-arg overload) and HasCount() (no-arg overload) that returned these wrappers have been removed. There is no longer any way to construct a LengthWrapper or CountWrapper, yet both files remain with only doc-comment updates applied.

These classes should be deleted outright. Keeping them adds noise to the public API surface (they are public) and the dead code signals an incomplete cleanup. Their presence also means the residual "IsTypeOf is not supported after HasCount()" error messages in CountWrapper are permanently stale — further evidence they should go.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() is too vague

protected override string GetExpectation()
{
    return "to satisfy length assertion";
}

This produces failure messages like: "expected to satisfy length assertion but value was null". A better approach would be to lazily capture the expectation from the inner assertion after evaluation — the inner Assertion<int> already has its own GetExpectation() which produces something like "to be equal to 5". Forwarding that gives: "expected length to be equal to 5 but received 3", which is much clearer. The CollectionCountWithInlineAssertionAssertion likely has the same gap.

3. InlineAssertionHelper is typed to int only

The helper is hard-coded to int. This works today since both count and length fit, but making it generic (ExecuteInlineAssertionAsync<T>) costs nothing now and avoids copy-paste if a long or double variant is needed later.

Minor

• StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string, while the count equivalent uses "collection was null". "string was null" would be more specific and consistent with the count wording.

Overall

The bulk of this PR — migrating 80+ callsites and removing the public obsolete API surface — is well-executed and complete. The main actionable items are deleting the now-unreachable CountWrapper/LengthWrapper classes and improving the GetExpectation() message in StringLengthWithInlineAssertionAssertion.

PR Review: Remove all [Obsolete] members and migrate callers — follow-up (3rd review)

Comparing against the previous two reviews.

What was addressed ✅

InlineAssertionHelper extraction + inner message propagation — confirmed present in commit 3cf4a1d. The shared helper deduplicates the count/length lambda-assertion execution and correctly propagates ex.Message from the inner assertion rather than replacing it with a generic fallback. This is the right design.

Third commit (96c7a43) test fixes — correctly updates tests to use Length(l => ...) where string context must be preserved for .And chaining, vs Length().IsEqualTo(N) where chaining ends. The comment in the commit message clearly explains the distinction.

Still outstanding from previous reviews

1. LengthWrapper and CountWrapper are unreachable dead code and should be deleted

Both HasLength() (no-arg, returning LengthWrapper) and HasCount() (no-arg, returning CountWrapper) have been removed. There is no longer any way to construct either class — yet both remain as public types with only their doc-comments updated.

Why this matters: They pollute the public API surface, and the NotSupportedException messages inside them (e.g. "IsTypeOf is not supported after HasCount()") are now permanently stale since HasCount() no longer exists. Leaving dead public types in an assembly that just did a deliberate API cleanup is at best confusing to users and future maintainers.

Recommendation: Delete both files.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() returns a vague message

This produces failure messages like: "expected to satisfy length assertion but value was null", which tells the user nothing about what was asserted. The inner Assertion<int> has its own GetExpectation() that would produce something like "to be equal to 5".

A better approach: run the lambda against a no-op source to extract the expectation lazily, or store a reference to the inner assertion and delegate to innerAssertion.GetExpectation() after evaluation. Compare with CollectionCountWithInlineAssertionAssertion — it likely has the same gap and both could be fixed together.

3. InlineAssertionHelper is hardcoded to int

This works today since both count and length are int. Making it generic costs nothing and keeps the helper reusable if a long, double, or TimeSpan inline assertion is ever needed:

Minor (unchanged from previous review)

• StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string. The count equivalent uses "collection was null". "string was null" would be more consistent with that wording pattern.

Overall

The removal work is thorough and correct — 80+ callsites migrated consistently. The three items above are the remaining actionable items, with deleting CountWrapper/LengthWrapper being the most important since those are now live dead code in the public API.

PR Review: Remove all [Obsolete] members and migrate callers — follow-up (3rd review)

Comparing against the previous two reviews.

What was addressed ✅

InlineAssertionHelper extraction + inner message propagation — confirmed present in commit 3cf4a1d. The shared helper deduplicates the count/length lambda-assertion execution and correctly propagates ex.Message from the inner assertion rather than replacing it with a generic fallback. This is the right design.

Third commit (96c7a43) test fixes — correctly updates tests to use Length(l => ...) where string context must be preserved for .And chaining, vs Length().IsEqualTo(N) where chaining ends. The comment in the commit message clearly explains the distinction.

Still outstanding from previous reviews

1. LengthWrapper and CountWrapper are unreachable dead code and should be deleted

Both HasLength() (no-arg, returning LengthWrapper) and HasCount() (no-arg, returning CountWrapper) have been removed. There is no longer any way to construct either class — yet both remain as public types with only their doc-comments updated.

Why this matters: They pollute the public API surface, and the NotSupportedException messages inside them (e.g. "IsTypeOf is not supported after HasCount()") are now permanently stale since HasCount() no longer exists. Leaving dead public types in an assembly that just did a deliberate API cleanup is at best confusing to users and future maintainers.

Recommendation: Delete both files.

2. StringLengthWithInlineAssertionAssertion.GetExpectation() returns a vague message

protected override string GetExpectation()
{
    return "to satisfy length assertion";
}

This produces failure messages like: "expected to satisfy length assertion but value was null", which tells the user nothing about what was asserted. The inner Assertion<int> has its own GetExpectation() that would produce something like "to be equal to 5".

A better approach: store a reference to the inner assertion after the lambda is evaluated, and delegate GetExpectation() to innerAssertion.GetExpectation(). This would produce messages like "expected length to be equal to 5 but received 3". The CollectionCountWithInlineAssertionAssertion likely has the same gap and both could be fixed together.

3. InlineAssertionHelper is hardcoded to int

The current signature is:

internal static async Task<AssertionResult> ExecuteInlineAssertionAsync(
    int actualValue,
    string label,
    Func<IAssertionSource<int>, Assertion<int>?> assertion)

This works today since both count and length are int. Making it generic costs nothing now and keeps the helper reusable if a long, double, or TimeSpan inline assertion is ever needed.

Minor (unchanged from previous review)

• StringLengthWithInlineAssertionAssertion.CheckAsync returns AssertionResult.Failed("value was null") for a null string, while the count equivalent uses "collection was null". "string was null" would be more specific and consistent with that wording.

Overall

The removal work is thorough and correct — 80+ callsites migrated consistently. The three items above are the remaining actionable items, with deleting CountWrapper/LengthWrapper being the most important since those are now live dead code in the public API.

PR Review: Remove all [Obsolete] members and migrate callers — follow-up (4th review)

Reviewing the two new commits since the last review: the merge commit and the "Regenerate public API snapshots after removing obsolete members" commit (df093be).

What the new commit does

The regenerated public API snapshots correctly reflect:

• HasCount() and HasLength() factory methods removed from all three target frameworks (net8.0, net9.0, net10.0)
• HasCount(int) and HasLength(int) removed
• ObjectBag, Timing, ITestOutput.Timings, RecordTiming removed from TUnit.Core
• StringLengthWithInlineAssertionAssertion added as a new public type

The snapshot regeneration is correct and necessary — good.

Still unresolved: CountWrapper and LengthWrapper remain as public dead code

The regenerated snapshots confirm what was flagged in the previous three reviews. Both classes still appear in the public API surface:

public class CountWrapper<TCollection, TItem> : ., .<TCollection>
    public CountWrapper(.<TCollection> context) { }
public class LengthWrapper : ., .<string>
    public LengthWrapper(.<string> context) { }

The only way to construct either of these was via HasCount() and HasLength(), which are now deleted. There is no constructor call path left — they are unreachable. Despite this, they remain public types visible to all consumers of TUnit.Assertions.

Why this matters: A future consumer browsing the assembly or IntelliSense will discover CountWrapper and LengthWrapper and have no way to use them. The NotSupportedException messages inside them reference HasCount() / HasLength(), which no longer exist, so the error guidance is permanently incorrect. Leaving them in the public API surface undermines the goal of this PR, which is specifically to clean up the public API.

Recommendation: Delete TUnit.Assertions/Conditions/Wrappers/CountWrapper.cs and TUnit.Assertions/Conditions/Wrappers/LengthWrapper.cs, then re-run snapshot tests to confirm they drop from the .verified.txt files.

Still unresolved: GetExpectation() in StringLengthWithInlineAssertionAssertion is vague

protected override string GetExpectation()
{
    return "to satisfy length assertion";
}

This produces failure messages like: "expected to satisfy length assertion but value was null". The inner Assertion<int> that the lambda returns has its own GetExpectation(). Delegating to it — after evaluating the lambda — would produce messages like "expected length to be equal to 5 but received 3", which is far more actionable for the test author. The CollectionCountWithInlineAssertionAssertion has the same gap. Both could be fixed together.

Still unresolved: InlineAssertionHelper hardcoded to int

internal static async Task<AssertionResult> ExecuteInlineAssertionAsync(
    int actualValue,
    string label,
    Func<IAssertionSource<int>, Assertion<int>?> assertion)

Making this generic (ExecuteInlineAssertionAsync<T>) costs nothing now and avoids duplication if a long, double, or TimeSpan inline assertion is ever needed. The current signature would require either a second copy of this method or a lossy int cast.

Overall

The obsolete API removal and snapshot regeneration are correct and complete. The three remaining items — dead CountWrapper/LengthWrapper classes, vague GetExpectation(), and the int-hardcoded helper — have carried through all four review iterations. The CountWrapper/LengthWrapper deletion is the most important because it directly impacts the exported public API surface that this PR set out to clean up.

Follow-up Review

All four concerns from the prior review have been addressed. Here's a summary of what was fixed and one remaining observation.

✅ Previous Issues — All Resolved

1. Duplication of inline-assertion execution logic — Fixed via the new InlineAssertionHelper.ExecuteInlineAssertionAsync<T>(). Both CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion now delegate to a single shared helper. This is exactly the right abstraction.

2. Inner failure message was silently swallowed — Fixed. Both assertions now store _innerAssertion and use _innerAssertion.InternalGetExpectation() in GetExpectation(), so failure messages reflect the actual constraint (e.g. "count to be equal to 5") rather than a generic fallback.

3. TimingEntry placement and #pragma suppressions — Clean. The internal record lives in TestContext.Output.cs, all obsolete CS0618 suppression pragmas are removed, and the TRX reporting path still works via the internal type.

4. "value was null" wording — Fixed to "string was null", consistent with the equivalent "collection was null" in the count assertion.

Minor Observation — InlineAssertionHelper propagates ex.Message as the failure detail

When resultingAssertion.AssertAsync() throws a TUnitAssertionException, ex.Message is the full formatted failure string (e.g. "Expected count to be equal to 5 but received 3"). That message then becomes the "received" portion of the outer assertion, while GetExpectation() returns "count to be equal to 5". The final failure message may read something like:

Expected count to be equal to 5 but count was: Expected count to be equal to 5 but received 3

Using just the raw value instead (e.g. AssertionResult.Failed($"{label} was {actualValue}")) would be cleaner — though this was already the pre-existing behaviour of Count(lambda), so it's not a regression introduced here. Worth a follow-up issue if it causes confusing output in practice.

Minor Observation — Inconsistency in test callsites for Length()

In OrAssertionTests.cs, Or_With_String_Assertions uses the new lambda overload:

While And_With_String_Assertions and Tests.cs use the existing chain form:

Both are valid, but the test suite would read more consistently if all migrated callsites used the same form. Not a blocking concern.

Overall

This is a well-executed cleanup PR. The breaking changes are appropriate (all removed members carried [Obsolete] warnings), the migration is complete and consistent, the abstraction improvement (InlineAssertionHelper) is the right call, and the public API surface snapshots are correctly updated. Good to go.

Follow-up Review

All four concerns from the prior review have been addressed. Here's a summary of what was fixed and one remaining observation.

All Previous Issues Resolved

1. Duplication of inline-assertion execution logic — Fixed via the new InlineAssertionHelper.ExecuteInlineAssertionAsync<T>(). Both CollectionCountWithInlineAssertionAssertion and StringLengthWithInlineAssertionAssertion now delegate to a single shared helper. This is the right abstraction.

2. Inner failure message was silently swallowed — Fixed. Both assertions now store _innerAssertion and use _innerAssertion.InternalGetExpectation() in GetExpectation(), so failure messages reflect the actual constraint (e.g. "count to be equal to 5") rather than a generic fallback.

3. TimingEntry placement and #pragma suppressions — Clean. The internal record lives in TestContext.Output.cs, all obsolete CS0618 suppression pragmas are removed, and the TRX reporting path still works via the internal type.

4. "value was null" wording — Fixed to "string was null", consistent with the equivalent "collection was null" in the count assertion.

Minor: InlineAssertionHelper propagates ex.Message as the failure detail

When resultingAssertion.AssertAsync() throws a TUnitAssertionException, ex.Message is the full formatted failure string (e.g. "Expected count to be equal to 5 but received 3"). That message then becomes the "received" portion of the outer assertion, while GetExpectation() returns "count to be equal to 5". The final failure message could read redundantly.

Using just the raw value (e.g. AssertionResult.Failed($"{label} was {actualValue}")) would be cleaner — though this was already the pre-existing behaviour of Count(lambda), so it's not a regression. Worth a follow-up issue if it causes confusing output in practice.

Minor: Inconsistency in test callsites for Length()

In OrAssertionTests.cs, Or_With_String_Assertions uses the new lambda overload (.Length(l => l.IsEqualTo(11))), while And_With_String_Assertions and Tests.cs use the chained form (.Length().IsEqualTo(11)). Both are valid, but the test suite would read more consistently if all migrated callsites used the same form.

Overall

This is a well-executed cleanup PR. The breaking changes are appropriate (all removed members carried [Obsolete] warnings), the migration is complete and consistent across ~60 callsites, the InlineAssertionHelper abstraction is the right call, and the public API snapshots are correctly updated. Good to go.